Skip to content

[jvm] Fix SAM#12903

Merged
kLabz merged 4 commits into
developmentfrom
jvm/SAM-fix
May 16, 2026
Merged

[jvm] Fix SAM#12903
kLabz merged 4 commits into
developmentfrom
jvm/SAM-fix

Conversation

@kLabz
Copy link
Copy Markdown
Contributor

@kLabz kLabz commented May 16, 2026

We cleaned up a bit too much #12901 ; I quickly found a runtime error after merging... 😅

kLabz added 4 commits May 16, 2026 18:15
…sition SAMs

Revert the AbstractCast/Common/Gctx parts of a134fd8. That commit
dropped the functional_interfaces_used hashtbl on the claim that the
genjvm AST scan in collect_used_functional_interfaces "strictly subsumes"
the AbstractCast writeback. It does not.

AbstractCast.do_check_cast's SAM branch unifies eright.etype against the
SAM signature and returns eright **unchanged** — no TCast wrapper. So
when a function expression is passed where a Java SAM is expected, the
SAM TInst never appears in the typed AST. The only place it exists is on
the extern callee's signature, which the scan deliberately skips
(`if not (has_class_flag c CExtern)`) to keep classpath noise out.

For TCall sites this is masked: the callee TField has a TFun etype whose
parameter types are visited, so the SAM TInst is reached transitively.
But TNew has no callee subexpression — the constructor's parameter types
live on c.cl_constructor.cf_type, which Type.iter never visits. A bound
instance-method reference passed to a Java extern's constructor (e.g.
`new TextToSpeech(this, onTtsInit)` against android.jar) therefore
produces a closure that does not implement the SAM interface, hitting
ClassCastException at runtime.

Restore the field on Common.context + Gctx.t (shared in Common.clone),
the AbstractCast writeback keyed by @:native path, and seed
collect_used_functional_interfaces's `used` hashtbl from it before the
AST scan runs.
…erence

The existing cases either bind to a typed local (SAM TInst lands in the
AST directly) or call a static method (TCall callee's TField has a TFun
etype that exposes parameter types to the scan). Neither exercises the
path that AbstractCast's functional_interfaces_used writeback is needed
for: a bound instance-method reference passed to a Java extern's
constructor, with the SAM type never named anywhere else in user code.

Add CtorOnly + CtorSam to test.Listeners and a Holder class in Main.hx
that does `new Listeners_CtorSam(onCtor, 42)`. Without the writeback the
emitted closure does not implement CtorOnly and the JVM throws
ClassCastException at runtime — same shape as RideAssist's
`new TextToSpeech(this, onTtsInit)` crash against android.jar.
…e in AST

Replace the functional_interfaces_used hashtbl plumbing (Common.context
+ Gctx.t + AbstractCast writeback + genjvm seeding) with a one-line
change in AbstractCast: wrap the function expression in mk_cast — a
TCast(eright, None) with the SAM type as the cast's etype.

The SAM TInst now lives in the typed AST at every conversion site, so
genjvm's existing collect_used_functional_interfaces scan finds it
naturally via note_fi_in_type on the TCast's etype. No cross-module
mutable state, no typing↔codegen back-channel, no @:native path-rewrite
bridging.

mk_cast is already the established pattern in this file — it's what the
MultiType abstract path uses a few lines up.
@kLabz kLabz marked this pull request as ready for review May 16, 2026 16:29
@Simn
Copy link
Copy Markdown
Member

Simn commented May 16, 2026

Is this actually a regression from #12901 though? In that case I'd be curious what exactly caused the behavioral change.

@kLabz
Copy link
Copy Markdown
Contributor Author

kLabz commented May 16, 2026

It's a regression from an earlier state of #12901 ; it worked fine before the cleanup, but it broke after that.

Edit: before #12901, that test would just not compile

@kLabz
Copy link
Copy Markdown
Contributor Author

kLabz commented May 16, 2026

That mkcast looks way better to me than the previous functional_interfaces_used solution.. making this PR basically a one line fix

@Simn
Copy link
Copy Markdown
Member

Simn commented May 16, 2026

I'm not sure because relying on the presence of a type of a cast expression seems a little brittle. It introduces the potential for the AST node to be optimized/mapped away which would then lead to subtle failures. The explicit lookup definitely looks more dorky, but it might also have been more robust.

But I don't have particularly strong feelings in either direction. As long as the tests cover this I'm fine with the change as-is.

@kLabz
Copy link
Copy Markdown
Contributor Author

kLabz commented May 16, 2026

Merging this, as current situation is bad (support has been added for SAM but it can crash at runtime). I'm using this feature a lot, I should notice early if such case can happen, and I would then go back to it and find a more robust solution.

@kLabz kLabz merged commit 2c58f5b into development May 16, 2026
61 checks passed
@kLabz kLabz deleted the jvm/SAM-fix branch May 16, 2026 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants